Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FBXLoader fix transform inheritance, convert back to local #20932

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

DVLP
Copy link
Contributor

@DVLP DVLP commented Dec 22, 2020

Related issue: #14520

Fix for node transformation inheritance in FBX loader

This contribution is funded by Bad.City.

@mrdoob mrdoob requested a review from looeee December 22, 2020 17:36
@mrdoob mrdoob added this to the r125 milestone Dec 22, 2020
@looeee
Copy link
Collaborator

looeee commented Dec 23, 2020

Thanks for the PR @DVLP. I'll review it after Christmas.

@looeee
Copy link
Collaborator

looeee commented Jan 12, 2021

@DVLP thank you for your patience. I've tested this with a bunch of models and this fixes a few outstanding problems (two open issues here, plus a couple of other problematic models I have kept for testing). Great work 😄

Fixes: #15287, #14204 (@makc can you confirm that one?)
Does not fix #14903.

@mrdoob mrdoob merged commit ff1a50b into mrdoob:dev Jan 12, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 12, 2021

Thanks!

@makc
Copy link
Contributor

makc commented Jan 14, 2021

makc can you confirm that one

I have no sources from back then, but I guess you could just drop the file into 3js editor? maybe thats what mrdoob did

@mrdoob
Copy link
Owner

mrdoob commented Jan 14, 2021

maybe thats what mrdoob did

Yep

@DVLP
Copy link
Contributor Author

DVLP commented Jan 15, 2021

Nice

@mrdoob
Copy link
Owner

mrdoob commented Jan 15, 2021

@DVLP Any ideas of what's left in #14204?

@DVLP DVLP deleted the FBXLoader-fix-parent-inheritance branch February 2, 2021 10:07
@DVLP
Copy link
Contributor Author

DVLP commented Feb 2, 2021

I thought about it and the first place to look at would be why lPostRotationM requires inverting with lPostRotationM.invert(). This is the part that is different compared to the source code from Autodesk CalculateGlobalTransform on this page . I'd start from investigating why is this required and searching earlier in the chain if it's unnecessarily inverted or the opposite, if it should be inverted earlier.

What I also noticed the other two rotations(lPreRotationM, lRotationM) were empty and inverting them didn't do anything so I left them without .invert. In other models they may not be empty so this could make a difference. Maybe lPreRotationM.invert() and lRotationM.invert() are also required?

That's where I'd start but currently I'm to busy to start debugging these.

Here's Blender implementation of this. Starts with a comment line # This is a nightmare.
https://github.com/blender/blender-addons/blob/efbcbb2665b7763463e58d1f36514c48bb43b44e/io_scene_fbx/import_fbx.py#L380

@looeee
Copy link
Collaborator

looeee commented Feb 2, 2021

This is a nightmare.

No arguments from me there 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants